FIX | Adds support for Shim gss api on Linux to delay loading libgssapi_krb… #65469

JRahnama · 2022-02-16T23:32:03Z

Ports dotnet/sqlclient#1411 to add shim gss api on Linux following changes in dotnet/runtime#55037

Summary

When working with Kerberos authentication on Unix with recent changes in Net6 users fail to get a connection from Sql server. There is no exception thrown as well. It just crashes.

Customer Impact

This will prevent users from upgrading to Net6 while using Kerberos authentication in Unix.

Testing

Adding tests require some special setup on CI pipelines to install Net6 TFS and enabled Kerberos authentication.

…5.so

ghost · 2022-02-16T23:32:12Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Ports dotnet/sqlclient#1411 to add shim gss api on Linux following changes in dotnet/runtime#55037

Summary

When working with Kerberos authentication on Unix with recent changes in Net6 users fail to get a connection from Sql server. There is no exception thrown as well. It just crashes.

Customer Impact

This will prevent users from upgrading to Net6 while using Kerberos authentication in Unix.

Testing

Adding tests require some special setup on CI pipelines to install Net6 TFS and enabled Kerberos authentication.

Author:	JRahnama
Assignees:	-
Labels:	`area-System.Net`
Milestone:	-

Wraith2 · 2022-02-17T00:17:23Z

src/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs

@@ -16,6 +16,9 @@ internal static partial class NetSecurityNative
            IntPtr bufferPtr,
            ulong length);

+        [DllImport(Interop.Libraries.NetSecurityNative, EntryPoint = "NetSecurityNative_EnsureGssInitialized")]


You probably want to use 'GeneratedDllImport' like the others in the file.

@JRahnama for context, this is new in 7.0. So you will need to use the new form in this change, and the back port will use the form you have here.

danmoseley · 2022-02-17T03:41:01Z

@wfurt you will remember this one. It was fixed in the Microsoft.SqlClient package but is hitting System.Data.SqlClient users trying to update to .NET 6. Can you confirm this if the right change? We will need to service it.

stephentoub · 2022-02-17T03:46:26Z

I've not followed what's broken and why, and there are no tests in the PR. What is SqlClient using that is now broken in. NET 6?

danmoseley · 2022-02-17T04:37:48Z

As well as the issue above the original discussion was was in #60906

wfurt · 2022-02-17T05:16:50Z

yes, I can look @danmoseley. In essence SqlClient uses our internals directly @stephentopub and some changes we did in 6.0 broke some assumptions. In general prior to 6.0 OpenSSL was always initialized and ready to be used but that is no longer true in 6.0. (we had other similar issues around that)

I think this is primarily for historical reasons from time when the code lived in corefx/runtime.
I created #62202 while back to create public API sufficient for external packages like SqlClient.

jkotas · 2022-02-17T06:19:29Z

src/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs

+        // dotnet runtime issue https://github.com/dotnet/runtime/pull/55037
+        static NetSecurityNative()
+        {
+            if (Environment.Version.Major >= 6)


This file is only included in .NET 7+ binaries in this repo, so this check is unnecessary.

And the EnsureGssInitialized calls are already being done in the main branch:

runtime/src/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.IsNtlmInstalled.cs

Lines 17 to 20 in 3806e15

static NetSecurityNative()

{

GssInitializer.Initialize();

}

. It is why you are getting the build breaks.

Perhaps you meant to create a PR against one of the servicing branches?

stephentoub · 2022-02-17T09:40:19Z

In essence SqlClient uses our internals directly @stephentopub and some changes we did in 6.0 broke some assumptions.

Thanks, that's what I thought the answer was going to be. The fix is for SqlClient to stop doing that, not for runtime to maintain its private implementation details as callable surface area. This has been known for years:
dotnet/SqlClient#303
If we want to "fix" it this one last time and it's easy and low cost to do, fine, but SqlClient really needs to be fixed instead moving forward.

Wraith2 · 2022-02-17T09:46:54Z

If we want to "fix" it this one last time and it's easy and low cost to do, fine, but SqlClient really needs to be fixed instead moving forward.

When I looked at the issue it wasn't clear to me what the fix actually is. If taking a copy of the used files into it's own repo isn't sufficient what is needed to decouple the two? It's also worth noting that there isn't going to be enough person power available to do anything even if it is high on the list of priorities (see #51836 (comment) ) so even if well understood the timeframe for doing this is years away.

stephentoub · 2022-02-17T10:02:58Z

When I looked at the issue it wasn't clear to me what the fix actually is.

it needs to stop P/Invoking into runtime's native code:
https://github.com/dotnet/SqlClient/tree/main/src/Microsoft.Data.SqlClient/netcore/src/Common/src/Interop/Unix
That could mean maintaining its own native library (potentially seeded from runtime's), it could mean taking a dependency on another library's public surface area for the needed functionality, etc. As it stands this is akin to private reflection.

Wraith2 · 2022-02-17T10:16:12Z

As it stands this is akin to private reflection.

Well I tried to stop it doing that as well if you read the issue I referred to and that won't happen either.
If there's no time to consume new apis until .net 8 it seems very unlikely that a replacement auth api can be forked in or independently developed.

As for this PR. I suspect the right thing to do is to find the original PR that fixes it in this repo and request a backport to the 6.0 release branch. Does that sound right?

jkotas · 2022-02-17T15:31:10Z

As for this PR. I suspect the right thing to do is to find the original PR that fixes it in this repo and request a backport to the 6.0 release branch. Does that sound right?

It does not sound right. The PR that introduced GssInitializer is part of release/6.0.

I do not think you actually need any fixes in the core runtime bits for this. If there is an actual problem to be solved, you should be able to keep hacking around it in SqlClient using more private reflection.

(And of course, the proper way to fix this once for all is to delete the dependency on runtime impl details from SqlClient.)

danmoseley · 2022-02-17T16:09:49Z

If that's the case then it would go in release/3.1 where this package ships from - and @JRahnama had it originally..

wfurt · 2022-02-17T16:31:10Z

it needs to stop P/Invoking into runtime's native code:

agreed. This is why I would like to add public surface for application to use Kerberos. (not the low-level)

JRahnama · 2022-02-17T17:45:48Z

Thanks every one for taking time and looking into this. We are investigating native interop removal from managed SNI in Microsoft.Data.SqlClient and it is in our to-do list, but this issue is still remaining in System.Data.SqlClient users with Net 6. Our original intent by making this PR and the other one in corefx was to unblock users from upgrading to Net6 at first step. I am not sure if any fix from M.D.SqlClient in this matter could be back ported to S.D.SqlClient. What would be the solution for that? As @danmoseley mentioned should we reopen the mentioned PR in release/3.1 in corefx?

wfurt · 2022-02-17T19:14:02Z

Perhaps we should chat off-line @JRahnama. As @jkotas mentioned the runtime changes are only in .NET 6 .e.g there should be no need to touch 5.x tor 3.x ... unless you want SqlClient from there to work with 6.0 runtime. Prior to 6.0 OpenSsl was initialized automatically during library load. That was changed in 6.0 to explicit call to support some particular scenarios.

Wraith2 · 2022-02-17T19:18:33Z

unless you want SqlClient from there to work with 6.0 runtime

I believe this is the goal.

jkotas · 2022-02-18T05:22:25Z

@danmoseley mentioned should we reopen the mentioned PR in release/3.1 in corefx?

I assume that you meant dotnet/corefx#43133. The delta in this PR affects both the core runtime assemblies and System.Data.SqlClient. The change in core runtime assemblies is unnecessary - it comes with unnecessary risk. If you go with adding the workaround to release/3.1, it should be authored to affect System.Data.SqlClient only.

JRahnama · 2022-02-22T17:38:27Z

Closing this PR and reopening the PR in corefx 3.1/release seems like the best option.

Adds support for Shim gss api on Linux to delay loading libgssapi_krb…

f4e6769

…5.so

JRahnama requested a review from wfurt February 16, 2022 23:32

dotnet-issue-labeler bot added the area-System.Net label Feb 16, 2022

ghost assigned JRahnama Feb 16, 2022

Wraith2 reviewed Feb 17, 2022

View reviewed changes

danmoseley mentioned this pull request Feb 17, 2022

[Release 3.1] | Address Shim gss api on Linux to delay loading libgssapi_krb5.so while using Net6 dotnet/corefx#43133

Merged

jkotas reviewed Feb 17, 2022

View reviewed changes

runfoapp bot mentioned this pull request Feb 18, 2022

System.Collections.Immutable.Tests on Wasm failing intermittently with Assertion at /__w/1/s/src/mono/mono/metadata/class.c:1990, condition `<disabled>' not met #65574

Closed

JRahnama closed this Feb 22, 2022

JRahnama deleted the shims-gss-api-linux branch February 22, 2022 17:38

ghost locked as resolved and limited conversation to collaborators Mar 24, 2022

karelz added this to the 7.0.0 milestone Apr 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX | Adds support for Shim gss api on Linux to delay loading libgssapi_krb… #65469

FIX | Adds support for Shim gss api on Linux to delay loading libgssapi_krb… #65469

JRahnama commented Feb 16, 2022 •

edited by danmoseley

Loading

ghost commented Feb 16, 2022

Summary

Customer Impact

Testing

Wraith2 Feb 17, 2022

danmoseley Feb 17, 2022

danmoseley commented Feb 17, 2022

stephentoub commented Feb 17, 2022 •

edited

Loading

danmoseley commented Feb 17, 2022

wfurt commented Feb 17, 2022

jkotas Feb 17, 2022

jkotas Feb 17, 2022

stephentoub commented Feb 17, 2022

Wraith2 commented Feb 17, 2022

stephentoub commented Feb 17, 2022 •

edited

Loading

Wraith2 commented Feb 17, 2022

jkotas commented Feb 17, 2022 •

edited

Loading

danmoseley commented Feb 17, 2022

wfurt commented Feb 17, 2022

JRahnama commented Feb 17, 2022 •

edited

Loading

wfurt commented Feb 17, 2022

Wraith2 commented Feb 17, 2022

jkotas commented Feb 18, 2022

JRahnama commented Feb 22, 2022

FIX | Adds support for Shim gss api on Linux to delay loading libgssapi_krb… #65469

FIX | Adds support for Shim gss api on Linux to delay loading libgssapi_krb… #65469

Conversation

JRahnama commented Feb 16, 2022 • edited by danmoseley Loading

Summary

Customer Impact

Testing

ghost commented Feb 16, 2022

Summary

Customer Impact

Testing

Wraith2 Feb 17, 2022

Choose a reason for hiding this comment

danmoseley Feb 17, 2022

Choose a reason for hiding this comment

danmoseley commented Feb 17, 2022

stephentoub commented Feb 17, 2022 • edited Loading

danmoseley commented Feb 17, 2022

wfurt commented Feb 17, 2022

jkotas Feb 17, 2022

Choose a reason for hiding this comment

jkotas Feb 17, 2022

Choose a reason for hiding this comment

stephentoub commented Feb 17, 2022

Wraith2 commented Feb 17, 2022

stephentoub commented Feb 17, 2022 • edited Loading

Wraith2 commented Feb 17, 2022

jkotas commented Feb 17, 2022 • edited Loading

danmoseley commented Feb 17, 2022

wfurt commented Feb 17, 2022

JRahnama commented Feb 17, 2022 • edited Loading

wfurt commented Feb 17, 2022

Wraith2 commented Feb 17, 2022

jkotas commented Feb 18, 2022

JRahnama commented Feb 22, 2022

JRahnama commented Feb 16, 2022 •

edited by danmoseley

Loading

stephentoub commented Feb 17, 2022 •

edited

Loading

stephentoub commented Feb 17, 2022 •

edited

Loading

jkotas commented Feb 17, 2022 •

edited

Loading

JRahnama commented Feb 17, 2022 •

edited

Loading